Skip to content

Conversation

@AlexeySachkov
Copy link
Contributor

We have a problem with our approach to XFAILing failed tests - we don't always submit trackers for those failures, thus simply hiding them from us.

Some of tests we XFAILed have been in that state for years and without a tracker submitted about them, we don't even know that there are issues.

This patch introduces a new requirement for marking test as expected to fail: every XFAIL directive has to be followed by XFAIL-TRACKER which points to intel/llvm issue submitted to analyze the failure and remove XFAIL directive.

The tracker can be referenced either by URL, or through GH shortcuts owner/repo#NNNNN.

To ensure that the new requirement is followed, a new test was added which checks amount of improper XFAIL directives (i.e. those which are not followed by XFAIL-TRACKER directive).

Similar approach is expected to be applied later for UNSUPPORTED directives, but it will be done as a separate PR.

We have a problem with our approach to `XFAIL`ing failed tests - we
don't always submit trackers for those failures, thus simply hiding them
from us.

Some of tests we `XFAIL`ed have been in that state for years and without
a tracker submitted about them, we don't even know that there are
issues.

This patch introduces a new requirement for marking test as expected to
fail: every `XFAIL` directive has to be followed by `XFAIL-TRACKER`
which points to intel/llvm issue submitted to analyze the failure and
remove `XFAIL` directive.

The tracker can be referenced either by URL, or through GH shortcuts
`owner/repo#NNNNN`.

To ensure that the new requirement is followed, a new test was added
which checks amount of improper `XFAIL` directives (i.e. those which are
not followed by `XFAIL-TRACKER` directive).

Similar approach is expected to be applied later for `UNSUPPORTED`
directives, but it will be done as a separate PR.
@AlexeySachkov AlexeySachkov marked this pull request as ready for review October 9, 2024 08:20
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner October 9, 2024 08:20
```
// GPU driver update caused failure
// XFAIL: level_zero
// XFAIL-TRACKER: intel/llvm#DDDDD
Copy link
Contributor

@KornevNikita KornevNikita Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - can't we proceed with the full link instead? Looks like I need to go to the "issues", open some and then paste the number to find the ticket (is there another way?)
upd. or I can use the search line. Anyway, It would be more convenient to have a link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format allows full links here and, it is just that example uses a shortcut. I agree that it will be easier to have a full link, so let me make it stricter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexeySachkov
Copy link
Contributor Author

Local merge highlighted that amount of XFAILed test changed, hence the update to the PR. I'm not entirely sure if it worth waiting for CI to complete, because I've verified the test locally and other patches with XFAILs could be merged while we are waiting for CI results here. Considering that the new test is only executed on Linux, I will cancel Windows workflows

@AlexeySachkov
Copy link
Contributor Author

Pre-commit passed, I just did a local merge merge with sycl and the test still passed, so I will proceed with merge of the PR. There could be some failures in the new test after I merge this - simply because someone passed pre-commit without this patch, but they should be addressed as a usual bugfix.

@AlexeySachkov AlexeySachkov merged commit ec57ad7 into intel:sycl Oct 11, 2024
10 of 12 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/track-amount-of-xfailed-tests-without-tracker branch October 11, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants